fix: use whitespace intersection for filtered text queries - #709
Conversation
vishal-bala
left a comment
vishal-bala
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis in #708 is exactly right, and the FT.EXPLAIN output showing and as a required term is what made this easy to act on. The two sites you picked are the two that matter most, and the new STOPWORDS 0 integration tests are the right shape: two documents differing only in the filtered field, so they actually distinguish "filter applied" from "filter ignored".
Two things I'd like to see before this merges. Both are on lines you're already touching, and both leave a real bug live otherwise.
The first is parenthesising the filter, which I've commented on inline. Worth noting your own issue proposed f" ({filter_expression})" — I think the issue had it right and the PR is the weaker version of it.
The second can't be an inline comment because the file isn't in this diff. There's a third AND join site: MultiVectorQuery._build_query_string in redisvl/query/aggregate.py has two of them.
383: range_query = " AND ".join(range_queries)
390: return f"({range_query}) AND ({filter_expression})"Line 383 is the same failure mode you diagnosed, on a STOPWORDS 0 index:
INTERSECT {
VECTOR { ... @v1 }
UNION { and, +and(expanded) }
VECTOR { ... @v2 }
...
}
Line 390 is easy to miss — grep '" AND "' doesn't match it, because the literal there is ) AND (. It needs the AND deleted separately; the groups are already parenthesised, so the parenthesisation change won't cover it. Both are one-line edits, and after them grep -rn ' AND ' redisvl/query/ redisvl/utils/ should come back empty, which seems like the right bar for a PR closing #708.
For scope: we have follow-up work queued for the remaining call sites — VectorQuery's KNN pre-filter (where a bare multi-clause pre-filter is a hard syntax error rather than silently wrong), VectorRangeQuery, and a shared helper so the join logic lives in one place instead of four. We'll open that as a downstream PR crediting this one, so please don't take any of it on here.
|
|
||
| if filter_expression and filter_expression != "*": | ||
| text += f" AND {filter_expression}" | ||
| text += f" {filter_expression}" |
There was a problem hiding this comment.
Could you wrap this in parentheses — text += f" ({filter_expression})", as your issue proposed?
Whitespace alone fixes the AND term but leaves a second precedence bug live. Redis binds | more loosely than whitespace intersection, so an unparenthesised multi-clause filter escapes the intersection. Unlike the AND bug, this one is not masked by the default stopword list, so it affects every user rather than only STOPWORDS 0 indexes:
FT.CREATE demo ON HASH PREFIX 1 e: SCHEMA text TEXT a TAG b TAG
HSET e:1 text "the quick brown fox" a x b n
HSET e:2 text "nothing here" a n b y
FT.SEARCH demo '@text:(fox) @a:{x} | @b:{y}' DIALECT 2 -> e:1, e:2
FT.SEARCH demo '@text:(fox) (@a:{x} | @b:{y})' DIALECT 2 -> e:1
e:2 contains neither fox nor a:{x}. FT.EXPLAIN on the first form shows why:
UNION {
INTERSECT { @text:fox, TAG:@a { x } }
TAG:@b { y }
}
To be clear, this is pre-existing rather than something you're introducing — on a default-stopwords index the old string parsed to the same plan once AND was dropped. But since the join is being rewritten anyway it's worth closing both at once, and the failure mode is nastier than the one you fixed: documents that match neither clause come back, which is harder to spot than zero results.
Only raw-string filters are affected. FilterExpression.format_expression already parenthesises composites, so (Tag("a") == "x") | (Tag("b") == "y") was never at risk.
|
|
||
| if filter_expression and filter_expression != "*": | ||
| query += f" AND {filter_expression}" | ||
| query += f" {filter_expression}" |
There was a problem hiding this comment.
Same parenthesisation here — query += f" ({filter_expression})".
The impact is lower than in TextQuery, because the text clause is ~-optional so INTERSECT{~T, X} returns the same document set as X whichever way the union binds. But the query plan is still wrong, and this helper is shared by both AggregateHybridQuery and the native HybridQuery, so I'd rather the two sites stay consistent than have one carry a latent precedence bug.
| query = TextQuery( | ||
| text="handbook", | ||
| text_field_name="text", | ||
| filter_expression=Tag("team") == "docs", |
There was a problem hiding this comment.
This filter is a single clause, so the test passes with or without the parenthesisation fix. Could you add one case with a union filter, so the precedence behaviour is pinned rather than incidental?
Something like filter_expression=(Tag("team") == "docs") | (Tag("team") == "legal") — though note that a FilterExpression union is self-parenthesising, so to exercise the actual bug the filter needs to be a raw string: filter_expression="@team:{docs} | @team:{legal}", with a third document that matches neither the text nor either tag. Without the fix that document comes back; with it, it doesn't.
vishal-bala
added
auto:release
|
🚀 PR was released in |
applied-ai-release-bot
Bot
added
the
released
2 participants
Fixes #708.
Summary
When an index is created without including
"and"as a stopword, the literal"AND"is included in the full-text query generated byTextQuery,AggregateHybridQueryandHybridQuery. This causes Redis Search to parseANDas an ordinary, fieldless full-text search term, returning no results even when documents satisfy both the text query and the supplied filter.Fixes
This PR modifies
TextQuery,AggregateHybridQueryandHybridQueryto use whitespace instead of"AND"to join full-text queries and filter expressions.Tests
Added regression tests covering filtered text and aggregate hybrid queries on an index created with stopwords disabled.
Note
Medium Risk
Changes Redis Search query syntax for all filtered text/hybrid/multi-vector paths; behavior should match Redis intersection semantics but any caller depending on the old
ANDstring could differ, especially on indexes without default stopwords.Overview
Fixes filtered full-text and hybrid searches on indexes where stopwords are disabled (e.g.
STOPWORDS 0). Generated query strings no longer insert the literal tokenANDbetween the text clause and a filter; they use whitespace intersection and parenthesize the filter instead (e.g.@text:(terms) (@team:{docs})).TextQuery,FullTextQueryHelper(shared byAggregateHybridQueryandHybridQuery), andMultiVectorQueryare updated consistently.MultiVectorQueryalso joins multiple vector range clauses with spaces rather thanAND.Integration tests cover filtered
TextQuery, raw-string union filters (operator precedence with|), and filteredAggregateHybridQueryon a no-stopwords index; unit expectations are aligned with the new syntax.Reviewed by Cursor Bugbot for commit 070a5b2. Bugbot is set up for automated code reviews on this repo. Configure here.